sequenceDiagram
    participant Client
    participant SQS Server
    participant Partitioner
    participant Visibility Store
    participant Data Store

    rect rgba(100, 150, 200, 0.5)
    Note over Client,Data Store: Partitioned FIFO Send
    Client->>SQS Server: SendMessage(MessageGroupId="group-A")
    SQS Server->>Partitioner: partitionFor(meta, "group-A")
    Partitioner-->>SQS Server: partition = 2
    SQS Server->>Data Store: Write(sqsMsgDataKeyDispatch(..., partition=2, ...))
    SQS Server->>Visibility Store: Write(sqsMsgVisKeyDispatch(..., partition=2, ...))
    SQS Server->>SQS Server: encodeReceiptHandleDispatch(..., partition=2, ...) → v2 handle
    SQS Server-->>Client: SendMessageResponse(ReceiptHandle=v2)
    end

    rect rgba(200, 100, 150, 0.5)
    Note over Client,Data Store: Partitioned Fanout Receive
    Client->>SQS Server: ReceiveMessage(MaxMessages=10)
    SQS Server->>SQS Server: readTS → startPartitionOffset → partition[0,1,2,3]
    loop For Each Partition
        SQS Server->>Visibility Store: Scan(sqsMsgVisScanBoundsDispatch(..., partition, ...))
        Visibility Store-->>SQS Server: Candidates{partition, visKey, messageID}
    end
    SQS Server->>Data Store: Load(sqsMsgDataKeyDispatch(..., cand.partition, ...))
    SQS Server->>SQS Server: encodeReceiptHandleDispatch(..., cand.partition, ...) → v2 handle
    SQS Server-->>Client: ReceiveMessageResponse(Messages=[...], ReceiptHandles=[v2])
    end

    rect rgba(150, 200, 100, 0.5)
    Note over Client,Data Store: Partition-Aware Delete
    Client->>SQS Server: DeleteMessage(ReceiptHandle=v2)
    SQS Server->>SQS Server: validateReceiptHandleVersion(meta, v2_handle)
    SQS Server->>Data Store: Delete(sqsMsgDataKeyDispatch(..., v2_handle.partition, ...))
    SQS Server->>Visibility Store: Delete(sqsMsgVisKeyDispatch(..., v2_handle.partition, ...))
    SQS Server-->>Client: DeleteMessageResponse()
    end

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

@claude review

Claude finished @bootjp's task in 8m 28s —— View job

Code Review: feat(sqs): wire partitioned-FIFO data plane through dispatch helpers (Phase 3.D PR 5b-2)

• Gather PR context and understand the request
• Read changed files (9 adapter files, ~900 additions / ~160 deletions)
• Analyze dispatch helpers and wiring logic
• Review test coverage
• Check for data loss, concurrency, and consistency issues
• Post complete review

Overall assessment

This is a well-structured, carefully staged PR. The core abstraction — a single dispatch layer (sqs_keys_dispatch.go) that routes to legacy or partitioned key constructors based on meta.PartitionCount — is the right shape, and the dormancy guarantee (no observable change on production queues) holds cleanly. The self-review in the PR description is thorough and accurate on all five CLAUDE.md lenses. There is one medium-severity finding and a few minor notes.

Medium: validateReceiptHandleVersion does not bound-check handle.Partition against meta.PartitionCount

File: adapter/sqs_keys_dispatch.go:165–196

validateReceiptHandleVersion verifies that the handle's version matches the queue's partition mode, but it does not verify that the partition index in a v2 handle is within [0, meta.PartitionCount).

A client that receives a legitimate v2 handle (partition = N) could forge a new v2 handle for the same queue with an out-of-range partition (e.g., partition = 63 on a 4-partition queue). After passing the version check, loadMessageForDelete constructs a key for partition 63, gets ErrKeyNotFound, and returns sqsDeleteNoOp — silently succeeding. This is AWS-idempotent but slightly incorrect semantics for a forged handle. More importantly, it leaks structural information: the client can confirm partition-key-space boundaries exist at that queue by observing the silent success vs. an explicit invalid-handle error.

// Suggested addition in validateReceiptHandleVersion, after the version checks:
if meta != nil && meta.PartitionCount > 1 && handle.Partition >= meta.PartitionCount {
    return errors.New("receipt handle partition out of range")
}

Since PartitionCount is immutable after queue creation (enforced by validatePartitionImmutability), this check is race-safe. The dormancy gate in PR 5b-2 means this can't be triggered today, but it should be in place before the gate is lifted in PR 5b-3.

Fix this →

Minor observations

1. now is computed per-partition in scanAndDeliverPartition (sqs_messages.go:901)

Before this PR, now was computed once in scanAndDeliverOnce. After, it is computed fresh at the start of each scanAndDeliverPartition call, meaning a message that crosses its visibility threshold between partitions 0 and 1 during a fanout receive might be delivered in partition 1's scan but would not have been found in a single-partition equivalent. This is strictly a behavior improvement (AWS does not guarantee anything stronger than eventual delivery anyway), but it is a subtle semantic delta worth noting in the PR description for future readers.

2. Integration test's legacy-keyspace probe end key uses 4 bytes of 0xFF (sqs_partitioned_dispatch_test.go:184)

end = append(end, 0xFF, 0xFF, 0xFF, 0xFF)

The actual message-ID suffix in the key is a base64url-encoded 16-byte UUID (~22 chars). The 4-byte 0xFF suffix works correctly because all base64url characters (A-Z, a-z, 0-9, -, _) have byte values < 0xFF, so any base64url string of any length compares less than [0xFF 0xFF 0xFF 0xFF]. However, using prefixScanEnd(legacyDataPrefix) (already available in the codebase at adapter/prefix_scan.go) would make the intent explicit and match the pattern used in the rest of the reaper code. Not a bug, just a consistency nit.

3. sqsMsgVisScanBoundsDispatch duplicates the scan-bounds logic from sqsMsgVisScanBounds (sqs_keys_dispatch.go:128–141)

The two functions share identical logic (construct prefix, zero-pad start, increment upper bound for end). A minor refactor opportunity: sqsMsgVisScanBoundsDispatch could delegate to sqsMsgVisScanBounds on the legacy branch rather than re-implementing the bounds math inline. The test TestSQSMsgVisScanBoundsDispatch_LegacyMatchesLegacy catches any divergence, so this is low risk either way.

Correctness verification (key paths)

Snapshot consistency: readTS is computed once in scanAndDeliverOnce and threaded identically through all partition fanout calls — consistent multi-partition snapshot. ✅

Group-lock partition coherence: partitionFor is deterministic (FNV-1a, power-of-two mask); a given MessageGroupId maps to exactly one partition for the lifetime of a queue generation. The group lock is always written and read at that same partition. ✅

PartitionCount immutability: validatePartitionImmutability is enforced on SetQueueAttributes, making the meta-dispatch pattern race-safe without re-loading meta inside OCC transactions. ✅

Dormancy preservation: Every code path that could emit a v2 handle is gated on meta.PartitionCount > 1, which CreateQueue continues to reject via validatePartitionDormancyGate until PR 5b-3. ✅

Test coverage

10 new tests are well-targeted. Particular highlights:

• TestSQSMsgVisScanBoundsDispatch_PerQueueOnPartitionedKeyspace directly pins the forward-note invariant from PR 731 round 2 (perQueue + partitioned keyspace must scan partitioned prefix, not legacy).
• TestSQSServer_PartitionedFIFO_SendReceiveDeleteRoundTrip sanity-checks the legacy keyspace is empty after all operations, which is the cleanest possible regression gate for future dispatch regressions.
• TestDecodeClientReceiptHandle_AcceptsV2 correctly documents the contract shift.

Missing: a test that a v2 handle with Partition >= meta.PartitionCount is rejected (tracking the medium finding above).

Verdict: Ready to merge after the partition bounds check is added to validateReceiptHandleVersion and covered by a test. The rest of the implementation is sound.

@claude review

Round 1 addressing Codex P1: rotate the receive-fanout starting partition by readTS so a sustained backlog on a single partition cannot starve higher-index partitions.

Changes:

• adapter/sqs_messages.go — new startPartitionOffset(partitions, readTS) helper. scanAndDeliverOnce now iterates (startOffset + i) % partitions. Implementation folds the upper and lower 32 bits of the HLC-derived readTS and mask-ANDs by partitions - 1 (power-of-two by validator invariant), keeping the computation in uint32 space (gosec G115 clean, no //nolint).
• adapter/sqs_partitioned_dispatch_test.go — TestStartPartitionOffset_RotatesByReadTS pins: legacy/perQueue → 0, every partition appears as a starting point across consecutive readTS, and the output is always in [0, partitions).

FIFO ordering: unaffected. partitionFor is deterministic, so a MessageGroupId always lands in one partition; cross-partition iteration order never reorders messages within any group.

Caller audit: scanAndDeliverOnce and scanAndDeliverPartition signatures and error semantics are unchanged — only the internal iteration order rotates. The new helper has one in-package caller.

P2/medium findings (out-of-range v2 partition validation, prefix-scan helper in test) are intentionally deferred — below the loop's P0/P1/high/critical/major threshold.

Claude finished @bootjp's task in 2m 22s —— View job

Code Review: Round 1 — Partition-starvation fix (startPartitionOffset)

• Gather PR context and understand the request
• Read adapter/sqs_messages.go — startPartitionOffset helper + fanout loop change
• Read adapter/sqs_partitioned_dispatch_test.go — TestStartPartitionOffset_RotatesByReadTS
• Review rotation logic correctness (gosec G115, power-of-two invariant)
• Review FIFO ordering preservation
• Review test coverage and edge cases
• Post complete review

Overall assessment

The fix is correct and well-scoped. The P1 finding (partition 0 starvation under sustained backlog) is addressed cleanly without touching any signatures or error semantics. Implementation and test match the stated intent. A few minor observations follow.

Correctness: startPartitionOffset

func startPartitionOffset(partitions uint32, readTS uint64) uint32 {
    if partitions <= 1 {
        return 0
    }
    var buf [8]byte
    binary.BigEndian.PutUint64(buf[:], readTS)
    folded := binary.BigEndian.Uint32(buf[0:4]) ^ binary.BigEndian.Uint32(buf[4:8])
    return folded & (partitions - 1)
}

Power-of-two invariant — PartitionCount is power-of-two enforced by htfifoValidatePartitionCount; effectivePartitionCount also returns 1 (= 2⁰) for the perQueue collapse, so mask-AND is equivalent to modulo throughout. ✅

gosec G115 avoidance — the byte-slice fold sidesteps the explicit uint64 → uint32 narrowing that gosec would flag. The approach correctly mirrors partitionFor. ✅

Distribution for real HLC values — the HLC layout is (physical_ms << 16) | logical_counter. Splitting at the 32-bit boundary gives:

• upper32 = physical_ms >> 16 (changes slowly, ~once per ms)
• lower32 = (physical_ms & 0xFFFF) << 16 | logical_counter (low 2 bits driven by logical_counter)

For consecutive receives in the same millisecond, logical_counter increments by 1 per call, so the low bits of folded increment by 1 → sequential cycling through all N partitions. ✅ For cross-millisecond boundaries, both halves change, maintaining spread.

Overflow safety in the loop — startOffset + i < 2 * partitions; with htfifoMaxPartitions = 32, max sum is 62, well within uint32. % partitions is safe since effectivePartitionCount guarantees partitions >= 1. ✅

Minor observations

1. Test exercises only low ts values where upper 32 bits are zero (sqs_partitioned_dispatch_test.go:335–353)

The coverage loop runs ts = 1..64 and ts = 0..1023. With these values, upper32 = 0, so folded = lower32 = ts, and the offset cycles trivially: ts & 3 = 0,1,2,3,0,.... This is correct but it doesn't exercise the XOR fold with non-zero upper bits. Real HLC timestamps have physical_ms >> 16 ≈ 0x190xxx (non-zero). Adding a sub-case with an HLC-range seed (e.g., ts = uint64(1_735_000_000_000) << 16) would give more confidence that the fold doesn't accidentally collapse to a constant offset for real values. The fix itself is still sound — this is a test quality observation only.

2. Comment verbosity (sqs_messages.go:907–925, sqs_messages.go:872–882)

CLAUDE.md's convention is: "one short line max" for comments. The startPartitionOffset doc block is ~10 lines and the inline loop comment is ~11 lines. The gosec-G115 reasoning is genuinely non-obvious and worth keeping, but both blocks could be compressed significantly. Not blocking, but the inline loop comment in particular restates what's already in the function's doc.

3. Loop uses % where mask-AND would be consistent (sqs_messages.go:892)

partition := (startOffset + i) % partitions

startPartitionOffset uses & (partitions - 1) and explicitly documents the power-of-two invariant. Using & (partitions - 1) in the loop too would be consistent and self-documenting, avoiding the % operator whose correctness requires knowing that partitions is never 0 (which it isn't, but the reader has to check). Either form is correct.

FIFO ordering

Preserved. partitionFor is deterministic (FNV-1a over MessageGroupId), so a group always hashes to exactly one partition regardless of which partition the fanout starts from. Re-ordering scanAndDeliverPartition calls cannot reorder messages within any group because cross-partition messages are by definition different groups. ✅

Summary

The P1 fix is correct, gosec-clean, and backward-compatible on every queue mode. The three observations above are all minor or nit-level. The implementation is ready to merge.